Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added recipe for the Prooph pack #320

Merged
31 commits merged into from
Oct 29, 2018
Merged

Added recipe for the Prooph pack #320

31 commits merged into from
Oct 29, 2018

Conversation

gquemener
Copy link
Contributor

@gquemener gquemener commented Mar 19, 2018

Q A
License MIT

Initial conversation is here

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request passes validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request passes validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request passes validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request passes validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request passes validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request does not pass validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request passes validation.

@Nyholm
Copy link
Member

Nyholm commented Mar 20, 2018

Let me know when you are ready for a review

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest updating travis.yml in a separate PR.

@gquemener could you address my other comments?

.travis.yml Outdated
@@ -25,4 +27,4 @@ install:
- composer config extra.symfony.allow-contrib true

script:
- composer req "${PACKAGES}"
- composer req $(echo "$PACKAGES" | tr --delete '\n')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not being clear. $PACKAGES will never contain more that one package.

@gquemener
Copy link
Contributor Author

Ok, for handling the travis configuration in a separate PR.

I'm taking care of your other comments, thanks.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request passes validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request passes validation.

@gquemener
Copy link
Contributor Author

It's done @Nyholm, thanks for your comments 👍

@ramondelafuente
Copy link

👍 Yay!

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Im happy with this PR. I left a small comment that you could fix if you want to.

I've tested this PR locally and it works great. To make sure it get merged, remove "WIP" from the title.

app.event_store.pdo_connection.mysql:
class: \PDO
arguments:
- 'mysql:host=%env(MYSQL_HOST)%;dbname=%env(MYSQL_DBNAME)%'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use a environment variable like "MSQL_DSN" here instead.

@Nyholm
Copy link
Member

Nyholm commented Jul 7, 2018

Any updates here?

@kochen
Copy link

kochen commented Jul 10, 2018

The php doc seems to answer no to this question: http://php.net/manual/en/ref.pdo-mysql.connection.php (it might be worth trying it though).

This indeed seems to be the case.

The factory method is a good idea indeed, but creates a dependency with the Doctrine bundle.

Then it's not worth fixing at the moment.

I suggest we go with what you have now and it needed/require we can always improve it later.

@ramondelafuente
Copy link

It looks like everyone in this thread is happy at this point... Is there anything else blocking this now? Or maybe it's a release cycle that it's waiting on? (just asking)

@gquemener
Copy link
Contributor Author

gquemener commented Sep 28, 2018

Hello 👋

Considering this summer announcement1 about dropping support on the prooph/service-bus package, I'd say this PR would need to be reworked before merged...

Any thought @UFOMelkor @codeliner?

Should it be simply closed?


1 here and here

@prolic
Copy link

prolic commented Sep 29, 2018 via email

@codeliner
Copy link

What @prolic said + the next service bus version could be a community driven project using symfony messenger but adding CQRS semantics on top of it. A future prooph pack could provide that. Anyway, service-bus is maintained until 31.12.2019.

@OskarStark
Copy link
Contributor

OskarStark commented Oct 29, 2018

So IIRC this one is ready to merge?

EDIT:
I mean from the recipe point of view (Travis needs to be fixed before ofc)

@fabpot
Copy link
Member

fabpot commented Oct 29, 2018

If that's the case, then the WIP in the title must be removed.

@gquemener
Copy link
Contributor Author

Travis is failing because this PR contains recipes for many packages at the same time. I had provided a solution, but @Nyholm asked me to revert it.

I am not sure what to do to fix the build then 🤔, would you have a suggestion @OskarStark ?

I'm removing the WIP flag

@gquemener gquemener changed the title [WIP] Added recipe for the Prooph pack Added recipe for the Prooph pack Oct 29, 2018
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request passes validation.

@ghost ghost merged commit 245e200 into symfony:master Oct 29, 2018
ghost pushed a commit that referenced this pull request Oct 29, 2018
@OskarStark
Copy link
Contributor

OskarStark commented Oct 29, 2018

Nice 👍

EDIT:
Oh this one was merged automatically, without a green travis build 🤔 @Nyholm can you please have a look at this?

@gquemener sorry, I missed your question. @Nyholm can you elaborate on this commit? as @gquemener said that there is more than one package in $PACKAGES

@OskarStark
Copy link
Contributor

The problem with travis could be, that you submitted many recipes in one PR and travis expects only one recipe per PR?

@gquemener
Copy link
Contributor Author

The problem with travis could be, that you submitted many recipes in one PR and travis expects only one recipe per PR?

Yes it is. The $PACKAGES var will contain "new line" delimited packages list which composer require don't understand (it needs a space delimited packages list).

I managed to get this space delimited list by using composer req $(echo "$PACKAGES" | tr --delete '\n')

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet